Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Screenshot file patterns (closes #1602, #1651, #1974, #1975) #2086

Closed
wants to merge 7 commits into from

Conversation

thireven
Copy link

@thireven thireven commented Jan 30, 2018

Starting this PR to begin discussions about this as well as address an issue I have with the current three test suites: I'm not able to pass all the tests from a fresh clone of testcafe. I'm on OSX 13.3.3 running Node v9.2.0. All three test suites fail to complete. I can create a separate bug issue for this if necessary.

I still need to write test cases for each of these patterns which I hope to start on after resolving my issue above.

Here is the documentation I've written so far for this new feature:

Patterns

Pattern Description
${FIXTURE} Fixture name
${TEST} Test name
${TEST_INDEX} Test index
${FILE_INDEX} Numbers a file in NNN format for files of the same name in the same folder
${DATE} Date the test was run (YYYY-MM-DD)
${TIME} Time the test was run (HH-mm-ss)
${DTF_<FORMAT>} Custom DateTime Format. Any supported MomentJS display format can be used here
${USERAGENT} Concatenation of ${BROWSER}, ${BROWSER_VERSION}, ${OS}, and ${OS_VERSION}
${BROWSER} Name of browser of current test
${BROWSER_VERSION} Browser version of current test
${OS} Name of operating system current test is currently run on
${OS_VERSION} Version of operating system current test is currently run on
${QUARANTINE_ATTEMPT} By default "1" (if you add it to the template but run tests without the -q option)

Usage & Examples

Set the pattern to be used with the -p or --screenshot-path-pattern flag.

testcafe chrome test1.js -s screenshots -p ${DATE}_${TIME}/test-${TEST_INDEX}/${USERAGENT}/${TEST_INDEX} (this pattern is the default one if no pattern is specified for backwards compatibility)

The pattern above reproduces the default file structure for the screenshot files:

└── 2018-01-30_13-32-47
    ├── test-1
    │   └── Chrome_63.0.3239_Mac_OS_X_10.13.3
    │       ├── 1.png
    │       └── thumbnails
    │           └── 1.png
    └── test-2
        └── Chrome_63.0.3239_Mac_OS_X_10.13.3
            ├── 2.png
            └── thumbnails
                └── 2.png

Patterns can also be used within the takeScreenshot() method like so: takeScreenshot('${TEST}/${OS}/${OS_VERSION}/${FILE_INDEX}'). Note that specifying a pattern in takeScreenshot() will override the pattern passed in the CLI for that specific screenshot.

Examples of patterns and their output:

${DATE}_${TIME}/${BROWSER}_${FILE_INDEX}_${FIXTURE}_${TEST}_${TEST_INDEX}

├── 2018-01-30_13-39-15
│   ├── Chrome_001_Screenshots-Pattern_Huehue_2.png
│   ├── Chrome_001_Screenshots-Pattern_Test_1.png
│   ├── Chrome_002_Screenshots-Pattern_Huehue_2.png
│   ├── Chrome_002_Screenshots-Pattern_Test_1.png
│   ├── Chrome_003_Screenshots-Pattern_Test_1.png
│   └── thumbnails
│       ├── Chrome_001_Screenshots-Pattern_Huehue_2.png
│       ├── Chrome_001_Screenshots-Pattern_Test_1.png
│       ├── Chrome_002_Screenshots-Pattern_Huehue_2.png
│       ├── Chrome_002_Screenshots-Pattern_Test_1.png
│       └── Chrome_003_Screenshots-Pattern_Test_1.png
${FIXTURE}/${TEST}_${FILE_INDEX}

└── Screenshots-Pattern
    ├── Huehue_001.png
    ├── Huehue_002.png
    ├── Test_001.png
    ├── Test_002.png
    ├── Test_003.png
    └── thumbnails
        ├── Huehue_001.png
        ├── Huehue_002.png
        ├── Test_001.png
        ├── Test_002.png
        └── Test_003.png
${FIXTURE}/${OS}_${BROWSER}/${TEST}/${FILE_INDEX}

── Screenshots-Pattern
    └── Mac-OS-X_Chrome
        ├── Huehue
        │   ├── 001.png
        │   ├── 002.png
        │   └── thumbnails
        │       ├── 001.png
        │       └── 002.png
        └── Test
            ├── 001.png
            ├── 002.png
            ├── 003.png
            └── thumbnails
                ├── 001.png
                ├── 002.png
                └── 003.png
${DTF_YYYY}/${DTF_MMMM+w}/${DTF_Do of MMM}/${TEST}

── 2018
    └── March+9
        └── 1st-of-Mar
            ├── Huehue.png
            ├── Test-2.png
            ├── Test.png
            └── thumbnails
                ├── Huehue.png
                ├── Test-2.png
                └── Test.png

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 82c465f have failed. See details:

@AlexanderMoskovkin
Copy link
Contributor

Hi @thireven,

Thank you for your contribution, we appreciate it a lot. Your proposal and current achievements look impressing!

IMPORTANT: The usage of %FILENUMBER% is important since screenshots will overwrite each other if they have the same name structure. %FILENUMBER% would automatically increment the name of the file to prevent such scenarios.

Maybe we shouldn't overwrite screenshots in this case because it can be unexpected for a person. I guess we can just add a numeral postfix for a file name in this case.

/cc @VasilyStrelyaev @DevExpress/testcafe Please take a look at this proposal, do you have some comments?

I have with the current three test suites: I'm not able to pass all the tests from a fresh clone of testcafe. I'm on OSX 13.3.3 running Node v9.2.0. All three test suites fail to complete.

Could you please clarify what errors do you have. Meanwhile we'll try to run in on our side with the same environment.

Also we have a "useragent" dependency in the project. You can use it to parse user agent.

@@ -89,6 +89,7 @@ export default class CLIArgumentParser {
.option('-b, --list-browsers [provider]', 'output the aliases for local browsers or browsers available through the specified browser provider')
.option('-r, --reporter <name[:outputFile][,...]>', 'specify the reporters and optionally files where reports are saved')
.option('-s, --screenshots <path>', 'enable screenshot capturing and specify the path to save the screenshots to')
.option('-p, --pattern <pattern>', 'screenshot files are named using specific patterns: %BROWSER%, %BROWSERVERSION%, %OS%, %OSVERSION%, %USERAGENT%, %DATE%, %TIME%, %FIXTURE%, %TEST%, %TESTNUMBER%, %FILENUMBER%')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe

use patterns to compose screenshot file names and paths: %BROWSER%, %BROWSERVERSION%, %OS%, %OSVERSION%, %USERAGENT%, %DATE%, %TIME%, %FIXTURE%, %TEST%, %TESTNUMBER%, %FILENUMBER%'

@AndreyBelym
Copy link
Contributor

AndreyBelym commented Feb 2, 2018

Unfortunately, we can't use %VARIABLE% syntax for placeholders, because Windows cmd.exe uses it for interpolating environment variables. Moreover, %DATE% and %TIME% are predefined variables. Also, rules of escaping aren't obvious (you need to put escaping character - ^ - not only before % signs, but also before quotes):

D:\testcafe-phoenix>node -e console.log(process.argv[1]) %DATE%      
Fri                                                                  
                                                                     
D:\testcafe-phoenix>node -e console.log(process.argv[1]) "%DATE%"    
Fri 02/02/2018                                                       
                                                                     
D:\testcafe-phoenix>node -e console.log(process.argv[1]) "^%DATE^%"  
^%DATE^%                                                             
                                                                     
D:\testcafe-phoenix>node -e console.log(process.argv[1]) ^"^%DATE^%^"
%DATE%                                                               

I think, we have to use shell-neutral template syntax, like Mustache. I would like to have standard JS template strings syntax: ${VAR}, but it will interfere with variable substitution on Unix shells (but for them you can just put single quotes around the pattern to disable substitution).

@thireven
Copy link
Author

thireven commented Feb 2, 2018

Could you please clarify what errors do you have. Meanwhile we'll try to run in on our side with the same environment.

From gulp test-server:

  1) CLI argument parser Should use "test" and "tests" dirs if source files are not specified:

      AssertionError: expected [ Array(4) ] to deeply equal [ Array(2) ]
      + expected - actual

       [
         "/Users/tnguyen/Projects/tc_test/test/server/data/file-list/test/test-dir-file.js"
         "/Users/tnguyen/Projects/tc_test/test/server/data/file-list/tests/tests-dir-file.js"
      -  "/Users/tnguyen/Projects/tc_test/test/server/data/file-list/Test/test-dir-file.js"
      -  "/Users/tnguyen/Projects/tc_test/test/server/data/file-list/Tests/tests-dir-file.js"
       ]

      at test/server/cli-argument-parser-test.js:308:36
      at <anonymous>

From gulp test-functional-local:

Running tests in browsers: chrome, ie, firefox
  1) "before all" hook

  0 passing (7s)
  1 failing

  1)  "before all" hook:
     TypeError: Cannot read property 'path' of null
      at checkBrowserPath$ (node_modules/testcafe-browser-tools/lib/api/open.js:34:33)
      at tryCatch (node_modules/testcafe-browser-tools/node_modules/babel-runtime/regenerator/runtime.js:72:40)
      at Generator.invoke [as _invoke] (node_modules/testcafe-browser-tools/node_modules/babel-runtime/regenerator/runtime.js:334:22)
      at Generator.prototype.(anonymous function) [as next] (node_modules/testcafe-browser-tools/node_modules/babel-runtime/regenerator/runtime.js:105:21)
      at tryCatch (node_modules/testcafe-browser-tools/node_modules/babel-runtime/regenerator/runtime.js:72:40)
      at invoke (node_modules/testcafe-browser-tools/node_modules/babel-runtime/regenerator/runtime.js:146:20)
      at bound (domain.js:280:14)
      at runBound (domain.js:293:12)
      at node_modules/testcafe-browser-tools/node_modules/babel-runtime/regenerator/runtime.js:191:11
      at new Promise (<anonymous>)

From gulp test-client:

[10:41:00] Starting 'fast-build'...
[10:41:00] Finished 'fast-build' after 31 μs
[10:41:07] Finished 'worker:lint' after 14 s
[10:41:07] Finished 'lint' after 15 s
[10:41:07] Starting 'build'...
[10:41:07] Finished 'build' after 10 μs
[10:41:07] Starting 'test-client'...
QUnit server listens on http://localhost:2000

It just hangs after the QUnit line and doesn't do anything else.

@AlexanderMoskovkin
Copy link
Contributor

So according to @AndreyBelym's proposal the template will look like

testcafe chrome test1.js -s screenshots -p "${DATE}_${TIME}/test-${TESTNUMBER}/${USERAGENT}/${TESTNUMBER}"

How do you like this?
/cc @thireven @VasilyStrelyaev @DevExpress/testcafe

@thireven
Copy link
Author

thireven commented Feb 5, 2018

test1.js -s screenshots -p >"${DATE}_${TIME}/test-${TESTNUMBER}/${USERAGENT}/${TESTNUMBER}"

I'm fine with that. Was also trying out using # as the delimiter over the weekend as well and it seems to work for OSX and Windows without any issues.

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit a31dc1b have failed. See details:

@miherlosev
Copy link
Collaborator

miherlosev commented Feb 6, 2018

I don't like this way.
Template string with separators has the following cons:

I propose to use a single function (packed as module) to build a custom screenshot path.
The function was named as CustomScreenshotPathProvider.

Example:

testcafe chrome -s screenshots -p OrganizationShare.js
testcafe ie -s screenshots -p DevFTP.js

Where OrganizationShare.js

/*Import necessary modules*/

export default function (runnerInfo, userAgent) {
    /*Custom logic*/
    return path.combine(runnerInfo.fixtureName, runnerInfo.testName, userAgent.browser...)
}

Pros of this way:

  • complex pattern is incapsulated to file with human-readable alias.
  • flexible and power (allows to use all nodejs modules)

What do you think about it?

@AlexanderMoskovkin
Copy link
Contributor

difficult to use conditional logic,

What conditional logic would you like in screenshot path template. I can't imagine a real-life case for that

potential problems with path separator char

It's not a problem. We normalize it by using path. We use the same way for test file path, screenshot dir path etc..

difficult to format datetime and useragent

There is not problems with useragent since we have ${BROWSER}, ${BROWSERVERSION}, ${OS}, ${OSVERSION}.

I see the only restriction here is it's impossible to format date. I'm not sure it's too important here. (if necessary we can add something like ${mm}, ${yyyy} later).

I feel the way with a separated js file is to complex and less native. I think it's not too nice to add a separated settings file to such specific feature. It possible we'll add a general settings file in future.

But I guess we should add such function to the programming api (is the same way as we did for runner.src). With this you can realize any logic there.

@AlexanderMoskovkin
Copy link
Contributor

AlexanderMoskovkin commented Feb 6, 2018

(UPDATED):

Additionally we already have some kind of templating for takeScreenshot action. See the Take Screenshot topic (UPD: I missed, actually we don't have. It just an example).

I guess we can just extend it for a whole test run (and avoid a breaking change there). So the API can see in the following way:

// CLI
testcafe chrome test.js -s <path> --screenshot-template "{currentDate}/test-{testIndex}/{userAgent}/{screenshotIndex}.png"

Available options:
- { date }
- { time }
- { testIndex }
- { screenshotIndex }
- { quarantineAttempt }

- { fixture }
- { test }
- { userAgent }
- { browser }
- { browserVersion }
- { os }
- { osVersion }
// Programming
runner.screenshot(path [, takeOnFails][, filePath])
// where filePath is a template string or a function that take the info and returns a value

How do you like this? /cc @VasilyStrelyaev @thireven @DevExpress/testcafe

@miherlosev
Copy link
Collaborator

What conditional logic would you like in screenshot path template. I can't imagine a real-life case for that

I don't want to invent user cases.
You option from previous comment covers known cases.
So, let it be.

@AlexanderMoskovkin
Copy link
Contributor

Additionally we already have some kind of templating for takeScreenshot action. See the Take Screenshot topic.

My bad. We don't have templating there yet. It's just a default value example in the Take Screenshot Topic. I've updated my comment a little bit. But the proposed API is still actual

@thireven
Copy link
Author

thireven commented Feb 9, 2018

Additionally we already have some kind of templating for takeScreenshot action. See the Take Screenshot topic.

Going off of what's there, would that mean that the pattern would change to {...} instead of ${...}? Would that run into any issues on various platforms?

@AlexanderMoskovkin
Copy link
Contributor

Hi @thireven,

After some considetaion let's stay with the following API:

-p or --screenshot-path-pattern

testcafe chrome test.js -s <path> -p "${DATE}_${TIME}/test-${TEST_INDEX}/${USERAGENT}/${FILE_INDEX}.png"

Available options:
- ${DATE}
- ${TIME}
- ${TEST_INDEX}
- ${FILE_INDEX}
- ${QUARANTINE_ATTEMPT} --by default "1" (if you add it to the template but run tests without the `-q` option).

- ${FIXTURE}
- ${TEST}
- ${USERAGENT}
- ${BROWSER}
- ${BROWSER_VERSION}
- ${OS}
- ${OS_VERSION}

If TestCafe creates a screenshot when a test fails (--screenshots-on-fails option enabled) we just add the -error postfix to the filename.

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 715b42f have failed. See details:

@thireven
Copy link
Author

thireven commented Mar 2, 2018

Made some updates according to what @AlexanderMoskovkin posted and added support for custom datetime formatting (DTF). The original PR info has been updated with information on DTF and an example pattern and output.

On the testing side, I'm still struggling to get two of them to succeed. gulp test-server now works and passes, but I still have the same issues with the other two.

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 48ee194 have failed. See details.

@testcafe-build-bot
Copy link
Collaborator

✅ Tests for the commit 48ee194 have passed. See details.

@AlexKamaev AlexKamaev closed this Mar 7, 2018
@AlexKamaev AlexKamaev reopened this Mar 7, 2018
@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 48ee194 have failed. See details:

this.quarantineAttemptNum = namingOptions.quarantineAttemptNum;
this.testIndex = namingOptions.testIndex;
this.screenshotIndex = 1;
this.errorScreenshotIndex = 1;

var testDirName = `test-${this.testIndex}`;
var screenshotsPath = this.enabled ? joinPath(this.baseScreenshotsPath, this.baseDirName, testDirName) : '';
var screenshotsPath = this.enabled ? this.baseScreenshotsPath : '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change leads to failures in test, see comment

@AlexKamaev
Copy link
Contributor

Hi

As you are using OSX, you do not have IE. But in the log I see the following:
Running tests in browsers: chrome, ie, firefox
and
at checkBrowserPath$ (node_modules/testcafe-browser-tools/lib/api/open.js:34:33)

So, I think it should work if you remove IE (and other missing browsers) in
..\test\functional\config.js

Simply modify the testingEnvironments[testingEnvironmentNames.localBrowsers] variable value

As for the gulp test-client, these tests work in a browser and it is required to open http://localhost:2000 in your browser.

I have also noticed that some AppVeyour tests failed.

145  1) [API] t.takeScreenshot() Should take a screenshot:
146
147      AssertionError: expected false to deeply equal true
148      + expected - actual
149
150      -false
151      +true
152      
153      at test\functional\fixtures\api\es-next\take-screenshot\test.js:18:88
154      at <anonymous>

Please take a look at this test.

it('Should take a screenshot', function () {
	return runTests('./testcafe-fixtures/take-screenshot.js', 'Take a screenshot', { setScreenshotPath: true })
		.then(function () {
			expect(SCREENSHOT_PATH_MESSAGE_RE.test(testReport.screenshotPath)).eql(true);  <---- failed here because of changes in comment
			expect(assertionHelper.checkScreenshotsCreated(false, 4)).eql(true);
		});
});

I have added a comment to the problematic code in the 'Files changed' tab

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit c5eeefe have failed. See details:

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 97d9034 have failed. See details:

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 77f4ddf have failed. See details:

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 6999614 have failed. See details:

@miherlosev
Copy link
Collaborator

This PR has intersections of the files changed with #2476.
I will continue to work after merge #2476.

@AndreyBelym AndreyBelym added this to the Sprint #13 milestone Jun 21, 2018
@miherlosev
Copy link
Collaborator

miherlosev commented Jul 4, 2018

It will take time to resolve all the conflicts. It'll be more convenient to create a separate PR.
So, I will close this PR.

@miherlosev miherlosev closed this Jul 4, 2018
@miherlosev miherlosev mentioned this pull request Jul 12, 2018
4 tasks
@MargaritaLoseva
Copy link
Contributor

I'm working on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants